Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shebang support with flakes #5189

Closed
wants to merge 8 commits into from
Closed

Conversation

tomberek
Copy link
Contributor

@tomberek tomberek commented Aug 28, 2021

Enables shebang usage of nix shell. All arguments with #! nix get
added to the nix invocation. This implementation does NOT set any
additional arguments other than placing the script path itself as the
first argument such that the interpreter can utilize it.

Example below:

    #!/usr/bin/env nix
    #! nix shell --quiet
    #! nix nixpkgs#bash
    #! nix nixpkgs#shellcheck
    #! nix nixpkgs#hello
    #! nix --ignore-environment --command bash
    # shellcheck shell=bash
    set -eu
    shellcheck "$0" || exit 1
    function main {
        hello
        echo 0:"$0" 1:"$1" 2:"$2"
    }
    "$@"

Overall it should make sense to any users of nix-shell.

  • add documentation
  • more tests
  • decide on defaults
    • automatically inject "shell" argument? (edit: nix run would work if it added PATHs)
    • re-use the "-i interpreter" concept from nix-shell?
    • lock file options?
    • whitelist/blacklist certain options
  • remove duplication of heuristic logic/code with nix-build
  • support a bootstrap mechanism detecting if nix is installed and fetching otherwise (eg nix —store or nix-portable)?
  • merge

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes

@roberth
Copy link
Member

roberth commented Aug 29, 2021

I really like this. I didn't think we could use nix because portable /usr/bin/env doesn't support extra arguments, but you've proven me wrong 🎉.

My only objection as far as the design is concerned, is that this isn't about shells at all, but on closer reading, this isn't specific to the dubious nix shell command at all. In fact, this seems to solve #801.

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments, but I really like the overall idea.

automatically inject "shell" argument?

I have a mixed feeling on that one: On the one hand, the only other command that (I think) could make sense in a shebang is nix develop, and that’s really an edge-case. On the other hand, having a too different behaviour when Nix is invoked as a shebang would probably be quite confusing (“why is ./myScript working but nix myScript returning some nonsense?”)

whitelist/blacklist certain options

I don’t think that would really be useful. At least from a security pov, you’re running an arbitrary script anyways, so restricting the interpreter wouldn’t change a lot.

support a bootstrap mechanism detecting if nix is installed and fetching otherwise (eg nix —store or nix-portable)?

How could that work? If you don’t have Nix installed, you can’t run the shebang at all afaik

src/libutil/args.cc Outdated Show resolved Hide resolved
if (runEnv && cmdline.size() > 0) {
auto script = *cmdline.begin();
try {
auto lines = tokenizeString<Strings>(readFile(script), "\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t know how much of an issue that is in practice, but this code reads the whole file, even when it bails right after because it doesn’t start by a shebang. Maybe you could use an FdSource or similar instead to only read as much as is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this? tomberek@cf02a4f

Could not see how to use FdSource for this use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that approach somewhat sane in a C++ world?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks alright.
The #! check is a bit complicated, but at least it won't suffer when the first "line" is very long (e.g. not a text file). When it looks like a shebang, you'll still want to ignore the whole line though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like tomberek@cf02a4f.
It would also be nice to have a way to have an explicit final nix shebang lines, so that we can stop scanning. This would allow for huge files to start with a nix shebang, which is useful for installers. Perhaps a line with #! nix-end-of-arguments or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another issue I’ve sometime hit is the desire to have a bit of shell parsing of the command line, or access to a SOURCE_SCRIPT var. (it’s an XY problem situation, I’m still trying to support perlPackages, etc)

src/libutil/args.cc Outdated Show resolved Hide resolved
@roberth
Copy link
Member

roberth commented Aug 31, 2021

To support nix develop, you could override the various basePath/baseDir variables to point to the script's location. That way its functioning does not depend on the caller's working directory, which could be anywhere.

@tomberek
Copy link
Contributor Author

Another issue that has come up as I'm writing the docs (stealing most of it from nix-shell), is that we don't run the setup or shell hooks to modify PYTHONPATH or PERL5LIB and so forth. Not sure if we want to adopt exactly the legacy nix-shell behavior which led us down an convoluted path. Is there an easier way?

@roberth
Copy link
Member

roberth commented Aug 31, 2021

we don't run the setup or shell hooks

This is why we need to rename the current nix shell to something like nix env run.
If you want to run stdenv, currently your only nix-command option is nix develop.

I would like Nix to defer to packages, allowing it to take "installing to environment variables" seriously. Maybe it's overkill because for regular commands PATH+XDG_DATA_DIRS will generally do, but perhaps this idea can be used for more complicated stdenv-like environment composition. Assuming Nix can somehow learn how to compose variables. (Not everything is :-separated)

Previously my (and apparently Eelco's) assumption was that we don't need the stdenv/setup-based ad-hoc shells like you are asking for. What's your use case? For packaging we've assumed writing an expression is best.

Anyway that's all fairly orthogonal to the shebang implementation, as long as we don't

automatically inject "shell" argument

I hope I was able to provide some context.

@roosemberth
Copy link

roosemberth commented Sep 2, 2021

   #!/usr/bin/env nix
   #! nix shell --quiet
   #! nix nixpkgs#bash
   #! nix nixpkgs#shellcheck

I find utterly confusing references to derivations are written as #! nix f#pkg instead of #! nix shell f#pkg or simply #! f#pkg.

I think we should also support a varadic derivation references until the end of the line (e.g. #! nix shell f#pkg1 f#pkg2) like nix shell already does.

@roberth
Copy link
Member

roberth commented Sep 2, 2021

The #! nix has the same role as #!nix-shell which seemed to have served a technical purpose for potentially mixing shebang aware interpreters. I wonder to what extend this is required. Perhaps it could be optional? For example, we could strip either #! or #!nix, whichever is a longer prefix. That way, we only need to confuse the reader when it's necessary for technical reasons.

Note that #! nix with the space would then be parsed as a #! prefix and a nix argument instead of #! nix as a prefix. I think this is an improvement, because #!nix reads a bit more like a single keyword.

Example:

#!/usr/bin/env nix
#! shell --quiet
#! nixpkgs#bash
#! nixpkgs#shellcheck

And in case we're invoking an interpreter that can and must ignore #!nix lines, but not #! lines.

#!/usr/bin/env nix
#!nix shell --quiet
#!nix nixpkgs#bash
#!nix nixpkgs#shellcheck
#! coolLang foo bar baz use imagination

@roosemberth
Copy link

roosemberth commented Sep 2, 2021 via email

@tomberek
Copy link
Contributor Author

tomberek commented Sep 2, 2021

The proposed approach is variadic and can be put all on one line. Sometimes these lines end up very long and having them split up can be helpful.

As to the prefix, this was meant to copy the way nix-shell does it( most of the code was simply copied or adopted over).

There are several shebang-aware interpreters, eg perl. I’m sure there are more.

The biggest issue is being unable to load the stdenv in the “right way”. I suspect that is actually what people will want. The examples of legacy nix-shell show bringing in python and Perl libraries and that will only work if we run shellHooks or at least do more than just modify PATH.

@ymeister
Copy link

ymeister commented Sep 8, 2021

Forgive me if I am wrong, but is it really necessary considering env -S option:

-S, --split-string=S
              process and split S into separate arguments; used to pass multiple arguments on shebang lines

That way your example becomes:

#!/usr/bin/env -S nix shell --quiet nixpkgs#bash nixpkgs#shellcheck nixpkgs#hello --ignore-environment --command bash
# shellcheck shell=bash
set -eu
shellcheck "$0" || exit 1
function main {
    hello
    echo 0:"$0" 1:"$1" 2:"$2"
}
"$@"

Aside from multi-line support, I don't see any problem here.

@roberth
Copy link
Member

roberth commented Sep 8, 2021

@ymeister that's a good suggestion, but it is not universally supported on the other unixes. It also doesn't allow spaces in arguments for use with --expr.

@tomberek
Copy link
Contributor Author

tomberek commented Sep 8, 2021

Forgive me if I am wrong, but is it really necessary considering env -S option

The original nix-shell shebang mechanism did not rely on that feature - i presume for various compatibility reasons. If one wishes to use it for their own scripts, they are welcome to do so. It should work with this PR (if not, I'd consider that a bug).

My greater concern is that examples such as (https://github.com/NixOS/nix/blob/7ab3bc32b139e251ff8d9abf25d6cadf3c0c6945/doc/manual/src/command-ref/nix-shell.md#use-as-a--interpreter) won't work with nix shell as it is currently implemented. Thus this is involved with the whole nix-shell/nix shell/nix develop/nix run/nix env do-a-thing design and debate. My summary of the situation is that there are a few use cases:

  1. Please put some packages A, B, C into my PATH. - usually nix-shell -p somepkg now nix shell
  2. I want to develop on package A. - usually nix-shell -A somepkg now nix develop
  3. Please make language-specific libraries available: eg: PYTHONPATH, PERL5LIB... - usually nix-shell -p or nix-shell shell.nix
  4. Just run a program, don't change any env vars - nix run

To make (1)(3) possible nix-shell would create a derivation behind the scenes on your behalf, composing it together and additionally invoking stdenv environment setup and shell hooks. This is why nix shell nixpkgs#python3 nixpkgs#python3Packages.requests does not work as expected. The shebang example from this PR hits this issue. The only way to express "please run the shellHooks" is to create your own flake devShell, but ad-hoc CLI mechanisms don't seem to allow it.

Another approach

  • Can we add "multiple installables" support to nix develop? Then nix develop is always the one that brings in stdenv/shellHooks.

This would create three distinct tools:

  • nix run - only runs an executable, no changes to env vars, multiple installables makes no sense
  • nix shell - only adds things to PATH, multiple installables means to add them all to the PATH
  • nix develop - add things to PATH and run shell hooks. multiple installables means to do a "buildEnv" composition.

@ymeister
Copy link

ymeister commented Sep 9, 2021

@roberth that's a good notice. Albeit the wide adoption of env -S among other unixes, it doesn't seem to be POSIX-compliant, so that's a bummer. On another note, I haven't experienced any problem using it with --expr:

#! /usr/bin/env -S nix shell --impure --expr "with import <nixpkgs> {}; python.withPackages (pkgs: with pkgs; [ prettytable ])" -c python

import prettytable

# Print a simple table.
t = prettytable.PrettyTable(["N", "N^2"])
for n in range(1, 10): t.add_row([n, n * n])
print t

(with import <nixpkgs> {} requires nixpkgs flake path in $NIX_PATH. That can be achieved either by pinning it system-wide (NixOS), or by exporting NIX_PATH="nixpkgs=$(nix flake metadata nixpkgs --json | jq -r .path)")

Above code also showcases an example of a workaround of the issue discussed by @tomberek. Although I agree that it is just a workaround and is nowhere near a proper solution like one proposed by @tomberek.

@roberth
Copy link
Member

roberth commented Sep 9, 2021

I may have been wrong about -S with quotes. I only read the man page, not the info page (:confused:) which goes into more detail, confirming your observations for at least GNU env and FreeBSD.
Instead of <nixpkgs> you should be able to reference a local flake when the base path is passed correctly. Also note that nix shell --expr is very different from nix-shell --expr (without and with stdenv respectively; also use case (1) vs (2))

@tomberek tomberek changed the title Shellbang support with flakes Shellbang support with flakes (blocked on nix run/shell/develop semantics) Nov 11, 2021
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-shell-with-shebang/17088/2

@Artturin
Copy link
Member

Please make sure these 2 keep working, Thanks.

#!/usr/bin/env nix-shell
//! ```cargo
//! [dependencies]
//! time = "0.1.25"
//! ```
/*
#!nix-shell -i rust-script -p rustc -p rust-script -p cargo
*/
fn main() {
    for argument in std::env::args().skip(1) {
        println!("{}", argument);
    };
    println!("{}", std::env::var("HOME").expect(""));
    println!("{}", time::now().rfc822z());
}
// vim: ft=rust
#!/usr/bin/env nix-shell
#![allow()] /*
#!nix-shell -i bash -p rustc
rsfile="$(readlink -f $0)"
binfile="/tmp/$(basename "$rsfile").bin"
rustc "$rsfile" -o "$binfile" --edition=2021 && exec "$binfile" $@ || exit $?
*/
fn main() {
    for argument in std::env::args().skip(1) {
        println!("{}", argument);
    };
    println!("{}", std::env::var("HOME").expect(""));
}
// vim: ft=rust

@chvp
Copy link
Member

chvp commented May 2, 2022

For anyone stumbling onto this in the future, you can avoid having to set NIX_PATH by using the following (as roberth hinted):

#!/usr/bin/env -S nix shell --impure --expr "(import (builtins.getFlake \"nixpkgs\") {}).python3.withPackages (ps: [ ps.requests ])" --command python

@fricklerhandwerk fricklerhandwerk added the UX The way in which users interact with Nix. Higher level than UI. label Sep 9, 2022
@fricklerhandwerk fricklerhandwerk added the feature Feature request or proposal label Sep 19, 2022
@roberth roberth assigned roberth and unassigned edolstra Jan 30, 2023
@github-actions github-actions bot added documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels May 1, 2023
@tomberek tomberek requested a review from roberth May 1, 2023 14:47
Comment on lines 86 to 95
auto isNixCommand = std::regex_search(programName, std::regex("nix$"));
if (isNixCommand && cmdline.size() > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a nice declaration would be

void Args::parseCmdline(const Strings & _cmdline, bool allowShebang = false);

This solves the architectural concern and a default is nicer than an overload I think.

@@ -69,6 +76,36 @@ void Args::parseCmdline(const Strings & _cmdline)
}

bool argsSeen = false;

// Heuristic to see if we're invoked as a shebang script, namely,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires quite a lot of refactoring in nix-build.cc, which seems disproportionate. I'm not convinced that the resulting abstraction would be useful, and coupling to the very stable nix-build.cc may well be counterproductive.

That said, factoring out some static functions / private methods will probably make the code easier to follow.

std::smatch match;
if (std::regex_match(line, match, std::regex("^#!\\s*nix\\s(.*)$")))
for (const auto & word : shellwords(match[1].str()))
cmdline.push_back(word);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation seems to strike a good balance. We may learn more in the experimental phase.
Resolving this thread.

src/libutil/args.cc Outdated Show resolved Hide resolved
Comment on lines +1490 to +1499
case '"':
cur.append(begin, it);
begin = it + 1;
st = st == sBegin ? sQuote : sBegin;
break;
case '\\':
/* perl shellwords mostly just treats the next char as part of the string with no special processing */
cur.append(begin, it);
begin = ++it;
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to comment on moved code, but I think shellwords isn't quite appropriate for this use case. I think we should fork it to be more predictable and future proof.
By restricting the syntax, we can be more compatible with the shell (in the shebang -> shell direction only of course; wouldn't want to reimplement a shell - in the limit). Enforcing this is perhaps convenient but most of all helps with understanding. Furthermore each syntax we forbid is trivially forward compatible.
No need to go overboard with this, so what I've tried to narrow it down to are the following changes based on shellwords:

  • In the unquoted state, reject single quotes. We don't need them and putting them in the parsed string would confuse readers.
  • Similarly, we should reject/reserve at least these chars in the unquoted state: \, $, `, <, >; perhaps also *.
  • Fail for lines that have an unterminated quoted string.
  • Use single quotes, because we won't be doing variable substitution (certainly for now) reject unescaped $ in double-quoted strings?

The backtick, <, and unterminated quoted string cases are particularly interesting, because they could serve as extension points for multiline strings, which I think would be very useful, but whose implementation could be scoped out of this PR.
OT?: I think double backticks would make a nice analog for nixlang's double single quote strings, except they'd be entirely verbatim.

These changes aren't compatible with nix-shell, so this would be new code and the move can be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roberth This is still pending.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think trying to conform with existing syntaxes and conventions is a far harder problem than coming up with a useful syntax that works well for our use case.
New proposal:

  • Use double backtick quotes as the only support type of quotation
  • Disallow any shell-like syntax outside the quoted strings, and be eager to reject syntax in general

This way we keep the code fairly simple, leaving reserved syntax for possible later extension, leaving our options open, while solving "our own use case" well: postponing good inline expression support seems like a mistake the more I think about it.

I'll have a go at implementing this.

```bash
#! /usr/bin/env nix
#! nix shell --impure --expr
#! nix "with (import (builtins.getFlake ''nixpkgs'') {}); terraform.withPlugins (plugins: [ plugins.openstack ])"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#! nix "with (import (builtins.getFlake ''nixpkgs'') {}); terraform.withPlugins (plugins: [ plugins.openstack ])"
#! nix 'with (import (builtins.getFlake "nixpkgs") {}); terraform.withPlugins (plugins: [ plugins.openstack ])'

This will then change a bit.

@fricklerhandwerk
Copy link
Contributor

Reviewed in Nix team meeting 2023-05-01:

  • status unclear
  • @roberth: stuck on syntax?
  • @edolstra: not a replacement for nix-shell -p yet
  • @roberth: that's ok, separate scope
  • @roberth: can technically be done with nix develop --expr '...'
  • @tomberek: what about the syntax? Currently scans all comment-like lines
  • @tomberek will rebase, @roberth still assigned for review

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-05-01-nix-team-meeting-minutes-51/27803/1

tomberek and others added 8 commits May 12, 2023 20:03
Enables shebang usage of nix shell. All arguments with `#! nix` get
added to the nix invocation. This implementation does NOT set any
additional arguments other than placing the script path itself as the
first argument such that the interpreter can utilize it.

Example below:

```
    #!/usr/bin/env nix
    #! nix shell --quiet
    #! nix nixpkgs#bash
    #! nix nixpkgs#shellcheck
    #! nix nixpkgs#hello
    #! nix --ignore-environment --command bash
    # shellcheck shell=bash
    set -eu
    shellcheck "$0" || exit 1
    function main {
        hello
        echo 0:"$0" 1:"$1" 2:"$2"
    }
    "$@"
```

fix: include programName usage
@roberth roberth force-pushed the shebang_flakes branch 2 times, most recently from f50bdfe to f8b54bf Compare May 12, 2023 18:39
@roberth roberth mentioned this pull request May 12, 2023
10 tasks
@roberth
Copy link
Member

roberth commented May 12, 2023

Oops, I didn't reset the upstream pushRemote. Made a separate PR with my additions. I'll revert this in a sec.
EDIT: github doesn't seem to tell me the old hash, or at least I'm not sure. I'll leave it at these rebased commits.

@roberth
Copy link
Member

roberth commented May 12, 2023

I've changed the quoting and interpretation of relative paths in #8327. wdyt?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/flox-nix-community-update-2023-04/28233/1

@tomberek
Copy link
Contributor Author

I've changed the quoting and interpretation of relative paths in #8327. wdyt?

Looks reasonable, but more contentious. I'm okay with either approach.

@musjj
Copy link

musjj commented Aug 10, 2023

Would it be desirable to have support for brace expansion? So you can do something like this:

#!/usr/bin/env nix
#!nix shell --command bash
#!nix nixpkgs#{bash,jq,fd}

@tomberek
Copy link
Contributor Author

v2 was merged, please try it out and provide feedback

@tomberek tomberek closed this Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation feature Feature request or proposal new-cli Relating to the "nix" command UX The way in which users interact with Nix. Higher level than UI. with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: Done
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.